TS was complaining about changing the shape of the object after creating it, so I moved it inline.

values from metric aggregations can be null too

If we don't explicitly tell which keys from aggregations are related to the chart, TS will create a union type of date_histogram and min/max buckets, and TS does not allow (AFAICT) maps over union types: microsoft/TypeScript#29011 (comment)

Sometimes TS needs a little help in realising object keys are strings (something to do with the keyof operator).

i can understand it...since numbers and symbols are valid object keys too, but in our case, it will always be strings :)

it's kind of weird to me that this is in a try/catch and silently fails when there is a HttpError. Anyone who knows why?

i don't know why it's like this, but i agree it seems strange. I would imagine that it might be this way so that a chart is simply omitted from rendering when there is an error, rather than displaying a chart + error messaging. But it seems like this is incomplete UX.

AggregationOption:

{
	name_of_agg: {
		terms: {
			...
		}
	}
}

That needs to be translated to:

{
	name_of_agg: {
		buckets: []
	}
}

That's what this part is for. It picks whatever key from name_of_agg that is an aggregation type, and uses that to get the response type from the parent object. Dirty, but works.

i don't know why it's like this, but i agree it seems strange. I would imagine that it might be this way so that a chart is simply omitted from rendering when there is an error, rather than displaying a chart + error messaging. But it seems like this is incomplete UX.

These type assertions could be obscuring type errors in the value. For chartBase, i like how we no longer have to define a special interface for chart-specific metrics. but it's now possible that a type mismatch for the rest of chartBase makes it thru to transformDataToMetricsChart via the type assertion. It might be safer for chartBase to be typed when it's defined.

A possible solution for best of both worlds:

const chartBaseSeries = {
  heapMemoryUsed: {
    title: i18n.translate('xpack.apm.agentMetrics.java.heapMemorySeriesUsed', {
      defaultMessage: 'Avg. used'
    }),
    color: theme.euiColorVis0
  },
  heapMemoryCommitted: {
    title: i18n.translate(
      'xpack.apm.agentMetrics.java.heapMemorySeriesCommitted',
      {
        defaultMessage: 'Avg. committed'
      }
    ),
    color: theme.euiColorVis1
  },
  heapMemoryMax: {
    title: i18n.translate('xpack.apm.agentMetrics.java.heapMemorySeriesMax', {
      defaultMessage: 'Avg. limit'
    }),
    color: theme.euiColorVis2
  }
};

const chartBase: ChartBase = {
  title: i18n.translate('xpack.apm.agentMetrics.java.heapMemoryChartTitle', {
    defaultMessage: 'Heap Memory'
  }),
  key: 'heap_memory_area_chart',
  type: 'area',
  yUnit: 'bytes',
  series: chartBaseSeries
};

export async function getHeapMemoryChart(setup: Setup, serviceName: string) {
  const result = await fetch<keyof typeof chartBaseSeries>(setup, serviceName);

  return transformDataToMetricsChart(result, chartBase);
}

with fetch defined as type:

export async function fetch<ChartBaseSeriesKey extends string>(
  setup: Setup,
  serviceName: string
): Promise<MetricSearchResponse<ChartBaseSeriesKey>> {
  ...

Yeah definitely looks better! I tried to make this work but ran into some issues when casting the return value to the return type. I've landed on a solution where instead of having several fetchers and fetch calls, we have a generic fetchAndTransformMetrics call with some parameters. fetchAndTransformMetrics then calls transformDataToMetricsChart, and TypeScript then - for some reason - is able to match the types together. Bonus is that we can merge a bunch of largely overlapping fetcher files into one file.

i can understand it...since numbers and symbols are valid object keys too, but in our case, it will always be strings :)

the type assertion as number[] is not needed here since it's inferred correctly

impact has a union type of number and null, and TypeScript can't figure out that we filtered out all non-null values in the filter function before it. I think in Lodash 4.x you can use compact to achieve the same effect, but the typings for 3.x are not complete enough.

this type assertion as Transaction should not be needed here since it inferred from _source

That works for the top-level hits object, but this is from a top_hits aggregation, so it needs a type hint. I'm not sure how to solve it without the type hint because it's so dynamic. Do you have any ideas?

in this case, i think you're right to use the type assertion